Skip to content

Use var instead of const in sanitizer to partially fix ci js tests #6354

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

echarles
Copy link
Member

@echarles echarles commented Apr 11, 2022

Partially fixes #6319 for the js-tests

Phantom.js does not seem to recognize const keyword. We just have to change that to var. No sanitizer behavior is changed with this PR.

@echarles
Copy link
Member Author

Once this gets merged, we can fix any remaining failing js-tests one by one. As the js-tests have not been maintained since some time, there will be a few to fix.

@blink1073 blink1073 added the bug label Apr 11, 2022
Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@echarles
Copy link
Member Author

echarles commented Apr 11, 2022

I have further investigated the js-test failure. The root cause is javascript flavor which is too new (e.g. es6, with let instead of var...) for the old webkit used by phantomjs. I have tried numerous ways/configs to generate old javascript flavors (see e.g. below).

The file which gives issues is the notebook/static/components/sanitizer/index created from @jupyterlab/apputils/lib/sanitizer/index. So even with this PR which fixes the base/security/index.js, phantomjs can not load correctly.

  module: {
    rules: [
      {
        test: /\.m?jsx?$/,
        use: {
          loader: 'babel-loader',
          options: {
            presets: [
              ['@babel/preset-env', {
                corejs: {
                  version: 2,
                  proposals: true
                },
                spec: true,
                modules: "umd",
                useBuiltIns: 'entry',
              }]
            ],
          }
        }
      }
    ]
  }

@blink1073
Copy link
Contributor

@echarles
Copy link
Member Author

Maybe try https://www.npmjs.com/package/string-replace-loader?

The issue is the generation of the sanitizer/index.js file which is done one time. Then the base/security.js is injected with that file... which can not be loaded by the headless phantomjs browser used by the js-test.

I have tried to tune webpack so it creates good-old javascript flavor without success.

How do you see the usage of string-replace-loader? As I understand it, it replaces so javascript modules by other modules, but does not really define the javascript flavor.

BTW webpack is not use at run-time, nor test-time AFAIK. It is just used to create the sanitizer/index.js, which itself is used at run-time and test-time.

@blink1073
Copy link
Contributor

True, it seems like something static is needed to run after the dev build to down-convert the JS.

@echarles
Copy link
Member Author

to down-convert the JS.

yeah, this is what I am looking for now, but maybe the needed downgrade is so large that modern babel can not do that anymore...

@blink1073
Copy link
Contributor

TypeScript also has a target option. You could write a shim file that gets transpiled by TS down to es5.

@echarles
Copy link
Member Author

Typescript target will act from ts->js. In our case, we already have the js in node_module/@jupyterlab/apputils/lib/sanitizer/index.js. @babel/preset-es2015 is deprecated and @babel/preset-env has to be used. I have update .babelrcto


{
  "presets": [
    [
      "env",
      {
        "targets": "node 5"
      }
    ]
  ]
}

Only var is generated (no more let), but still casperjs notebook/static/components/sanitizer/index.js fails with

SyntaxError: Unexpected token ')'

  phantomjs://code/index.js:1 in injectJs
  phantomjs://code/bootstrap.js:435

@echarles
Copy link
Member Author

echarles commented Apr 11, 2022

I am turning round and round since hours. If casperjs can not read the generated notebook/static/components/sanitizer/index.js whatever format we define... a workaround would be to run the tests on the old sanitizer based on google-caja. Just for the sake of the test as workaround to be able to run the CI tests... the runtime sanitizer would remain the current one based on jupyterlab. WDYT?

https://github.com/jupyter/notebook/pull/6340/files

@blink1073
Copy link
Contributor

WDYT?

Works for me, as long as the sanitizer tests themselves run with the newer code.

@echarles
Copy link
Member Author

Closing in favor of #6356

@echarles
Copy link
Member Author

Thx a lot @blink1073 for the help and ideas to find a solution.

@echarles echarles closed this Apr 12, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants